add extend_front to VecDeque with specialization like extend (unfinished) #146861
Conversation
rustbot
added
S-waiting-on-author
workingjubilee
added
T-libs-api
|
cc @the8472 a bunch of questions re: specialization that I believe you may be able to address |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Why are certain things behind feature gates, cfg(not(test))
This is due to tests living in the alloctest crate and some of the tests need access to implementation details so they're done with #[path] module imports there which means the originals need to be disabled.
See #136642
cfg(not(no_global_oom_handling)) like
This one is an unstable feature previously used by the linux kernel. IIRC a few other projects are using it too. It's supposed to remove all methods that abort on oom rather than returning an error from alloc's API surface.
Feature was requested in #69939
It's recommended to file an API change proposal to get some preliminary review of the proposed API from the team.
Considering that there are open questions how the API should look like I think it'd be best to have that discussion on an ACP thread so we can focus on the implementation details there.
| } | ||
|
|
||
| #[cfg(not(test))] | ||
| impl<T, A: Allocator> SpecExtendFront<T, Rev<vec::IntoIter<T>>> for VecDeque<T, A> { |
There was a problem hiding this comment.
This is a curious specialization. Is the expectation that people will frequently use Rev to prepend to a vecdeque?
Anyway, specializations should have dedicated tests since they're not covered by the regular ones.
There was a problem hiding this comment.
This is a curious specialization. Is the expectation that people will frequently use
Revto prepend to a vecdeque?
This depends on the design of extend_front, if it will act like repeated push_front, specializing this can have significant performance improvements. This would be used when prepending elements from a Vec into a VecDeque, in original order, a bit like the requested VecDeque::prepend (#69939 (comment)).
There was a problem hiding this comment.
prepend (see tracking issue) will use this, it calls self.extend_front(other.into_iter().rev())
I don't think any of the questions are specific to specialization? |
This comment has been minimized.
This comment has been minimized.
antonilol
force-pushed
the
vec_deque_extend_front
branch
from
a0a54e3 to
6da1b60
Compare
| // impl<T, A: Allocator> SpecExtendFront<T, Rev<Copied<slice::Iter<'_, T>>>> for VecDeque<T, A> | ||
| // where | ||
| // T: Copy, | ||
| // { | ||
| // #[track_caller] | ||
| // fn spec_extend_front(&mut self, iter: Rev<Copied<slice::Iter<'_, T>>>) { | ||
| // unsafe { prepend(self, iter.into_inner().it.as_slice()) }; | ||
| // } | ||
| // } |
There was a problem hiding this comment.
I tried to add specialization for deque.extend_front(slice.iter().rev()) where slice is &[T] with T: Copy (thus also deque.prepend(slice.iter())) but got stuck here. Copied's inner iterator can not be accessed (I made it pub while trying) and I get the following error: "cannot specialize on trait Clone". Any ideas on if and how this is possible? What does Extend do different so that it can specialize on Copy?
This comment has been minimized.
This comment has been minimized.
antonilol
force-pushed
the
vec_deque_extend_front
branch
2 times, most recently
from
22995ae to
552e6f1
Compare
antonilol
force-pushed
the
vec_deque_extend_front
branch
from
5c9b311 to
d4ae120
Compare
rustbot
added
the
S-waiting-on-review
rustbot
removed
the
S-waiting-on-author
6 participants
ACP: rust-lang/libs-team#658
Tracking issue: #146975
Text below was written before opening the ACP
Feature was requested in #69939, I recently also needed it so decided to implement it as my first contribution to the Rust standard library. I plan on doing more but wanted to start with a small change.
Some questions I had (both on implementation and design) with answers:
extendallows iterators that yield&TwhereTisClone, should extend_front do too?A: No, users can use
copiedand/orcloned.VecDeque?A: No, see ACP.
extend, maybe there is a nice way to factor out the code aroundpush_unchecked/push_front_unchecked.Will come back to this later.
cfg(not(test))likevec::IntoIterhere andcfg(not(no_global_oom_handling))likeVec::extend_from_within? (I am also looking at implementingVecDeque::extend_from_within)A: See add extend_front to VecDeque with specialization like extend (unfinished) #146861 (review)
extend_frontact like repeated pushes to the front of the queue? This reverses the order of the elements. Doing it different might incur an extra move if the iterator length is not known up front (where do you start placing elements in the buffer?).A:
extend_frontacts like repeated pushes,prependpreserves the element order, see ACP or tracking issue.